Bug 1931505: [on-prem] Cleanup keepalived vips before starting service#2511
Conversation
|
/test e2e-openstack |
kikisdeliveryservice
changed the title
There was a problem hiding this comment.
Shouldn't you specify also the netmask here?
I checked it in my environment and I got [1] warning
[1]
- remove_vip 192.168.111.5
- address=192.168.111.5
++ ip -o a
++ grep 192.168.111.5
++ awk '{print $2}' - interface=enp2s0
- '[' -n enp2s0 ']'
- ip a del 192.168.111.5 dev enp2s0
Warning: Executing wildcard deletion to stay compatible with old scripts.
Explicitly specify the prefix length (192.168.111.5/32) to avoid this warning.
This special behaviour is likely to disappear in further releases,
fix your scripts!
There was a problem hiding this comment.
Yes, you're right. I had mistakenly hard-coded the mask in my first version of this patch, and didn't notice that it triggered the warning after I removed it. The latest version should handle it correctly.
cybertron
force-pushed
the
keepalived-vip-cleanup
branch
from
7a89a54 to
a156dc2
Compare
|
/test e2e-openstack I really wish Prow didn't forget about the explicitly run jobs every time you update a PR. |
There was a problem hiding this comment.
If we intend to someday remove the workaround then we should reference the upstream keepalived issue. I personally don't see a problem keeping the removal of the VIP in the startup script forever, so up to you if you want to modify the patch.
There was a problem hiding this comment.
In the interest of not throwing away the ci passes I have I think I'll leave it alone for now. I can push a small followup to add a reference to the bz that can be merged whenever. If that doesn't make 4.8 it won't be a big deal.
There was a problem hiding this comment.
@cybertron Maybe we should add that now, since it had to be rebased in the last 15 minutes anyway?
There was a problem hiding this comment.
Yep, good point. Thanks for the reminder.
|
/lgtm |
|
Since this PR suppose to fix [1] BZ, maybe it worth link it to BZ? |
|
/lgtm |
I intentionally didn't do this because it doesn't fix the underlying bug, but maybe we could open a separate one against keepalived itself? |
Adding approval and a hold, given the above ^^ @cybertron if you want to add a BZ, please do so and remove the hold when you are ready. /approve |
openshift-ci-robot
added
do-not-merge/hold
Thanks, I think I will. It's possible we will need to backport this too since the bug has existed for a while. I'm not sure why it only became a problem in 4.8, but it could easily pop up in earlier releases too. |
cybertron
changed the title
|
@cybertron: This pull request references Bugzilla bug 1931505, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (vvoronko@redhat.com), skipping review request. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
bugzilla/severity-high
|
Okay, I opened a new bug against keepalived for this and added the existing one to this PR. Once we get ci passes this should be good to go. /test e2e-vsphere |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cybertron, Gal-Zaidman, jcpowermac, kikisdeliveryservice, mandre, yboaron The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-openstack |
|
/test e2e-openstack |
1 similar comment
|
/test e2e-openstack |
|
Can we remove the hold? Looks like the relevant things are passing \o/ |
|
/hold cancel Yep, I think we should be good to go now. |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/cc |
|
/retest |
|
/retest One down... |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
vsphere-upgrade and okd tests are not expected to pass |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/test e2e-agnostic-upgrade |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@cybertron: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/retest |
|
/refresh |
1 similar comment
|
/refresh |
|
@cybertron: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/skip |
|
@cybertron: All pull requests linked via external trackers have merged: Bugzilla bug 1931505 has been moved to the MODIFIED state. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
12 participants
Due to a bug in keepalived 2.0.10, if the liveness probe kills
keepalived, any vips that were assigned to the system remain and
are not cleaned up when keepalived restarts. Until we get a fixed
version of keepalived, let's clean up the VIPs ourselves before
starting keepalived. If the node is supposed to have the VIP it will
configure it again when keepalived starts.
This is a workaround for https://bugzilla.redhat.com/show_bug.cgi?id=1931505
- Description for the changelog
Under some circumstances, a VIP may be configured on a node it shouldn't be due to keepalived exiting uncleanly. To avoid this, remove any existing VIPs before starting keepalived.